Skip to content

Scan-skip test goes blind if the call chain grows 3 frames (#707) - #708

Open
philcunliffe wants to merge 4 commits into
masterfrom
fix/issue-707
Open

Scan-skip test goes blind if the call chain grows 3 frames (#707)#708
philcunliffe wants to merge 4 commits into
masterfrom
fix/issue-707

Conversation

@philcunliffe

Copy link
Copy Markdown
Contributor

Root cause

test/core/cache-retention-maintenance.test.js, test a partition already due for compaction skips the resettle-candidate row scan, attributes each .parquet readFileSync to its caller by capturing new Error().stack inside a mock and asserting no stack passes through hasResettleCandidate.

The discriminating frame sits at frame 8 of 10, and Error.stackTraceLimit defaults to 10:

Error
    at Object.<anonymous> (test/core/cache-retention-maintenance.test.js:1076:55)
    at Object.apply (node:internal/test_runner/mock/mock:765:20)
    at Object.reader (src/core/cache/iceberg/resolver.js:26:26)
    at readDataFile (node_modules/icebird/src/read.js:182:35)
    at readDataFile.next (<anonymous>)
    at Object.rows (node_modules/icebird/src/sql/icebergDataSource.js:170:30)
    at async scanRowsFromTable (src/core/cache/iceberg/store.js:509:20)
    at async hasResettleCandidate (src/core/cache/maintenance.js:829:22)   <-- frame 8
    at async maintainGeneration (src/core/cache/maintenance.js:302:32)
    at async withSpan.component (src/core/cache/maintenance.js:154:16)

Two frames of headroom. If the chain between the mock and hasResettleCandidate gains 3 or more frames (an icebird refactor, extra node:test mock internals, a wrapper in resolver.js), the frame silently falls off and the negative assertion stacks.filter(s => s.includes('hasResettleCandidate')) passes vacuously - the test stays green even with the !compactionDue && gate reverted.

The existing stacks.length > 0 sanity check only proved a read happened, not that stacks were attributable, and the new Error().stack ?? '' fallback would store an unattributable empty string without failing.

The fix

Both hardenings suggested on the issue, because they cover different routes to the same blindness:

  1. Raise Error.stackTraceLimit to 50 while the mock is installed, restoring the previous value in the existing finally so it is put back even if the test throws. This removes the practical hazard outright.
  2. Positive attribution assertion: some captured stack must name compactGeneration, the legitimate reader. It calls scanRowsFromTable from exactly the same depth as hasResettleCandidate does, so any truncation deep enough to hide the frame the negative assertion hunts for also hides this one, and the test fails loudly instead of going quiet. Asserting on scanRowsFromTable itself would not work: it sits one frame shallower and survives truncation that has already blinded the real check. This guard also catches the ?? '' empty-string fallback.

The existing comment's explanation of why a mock-based observation is the only option (hasResettleCandidate is module-private, scanRowsFromTable is an unpatchable ESM named import) is kept, and extended to describe the two new guards.

No production code changed.

Demonstration

Truncation is simulated with a preload module (Error.stackTraceLimit = 7, standing in for three extra frames) passed via NODE_OPTIONS="--import ...".

Step 0 - the test is sensitive today at the default limit

Gate reverted (const hasResettle = settle), no truncation:

not ok 1 - a partition already due for compaction skips the resettle-candidate row scan
  error: 'the resettle scan must not read the data file'
  ...
      at async hasResettleCandidate (src/core/cache/maintenance.js:829:22)
# pass 0
# fail 1

Step 1 - blindness reproduced

Gate still reverted, plus truncation. Old test:

TAP version 13
# Subtest: a partition already due for compaction skips the resettle-candidate row scan
ok 1 - a partition already due for compaction skips the resettle-candidate row scan
# pass 1
# fail 0

Green with the gate reverted. Exactly the defect the issue describes.

Step 2 - hardening applied

(the diff in this PR)

Step 3 - hardened test fails under the same truncation, gate still reverted

Guard 1 (the limit raise) defeats the external truncation, so the real check sees the frame again and fires:

not ok 1 - a partition already due for compaction skips the resettle-candidate row scan
  error: |-
    the resettle scan must not read the data file
# pass 0
# fail 1

Step 3b - guard 2, checked independently

Simulating truncation that the raise cannot fix (in-test limit forced to 7, as if the chain had outgrown even 50), gate still reverted. The positive assertion fires rather than the test going quiet:

not ok 1 - a partition already due for compaction skips the resettle-candidate row scan
  error: 'sanity: captured stacks must be deep enough to name the reader, or the assertion below passes vacuously'
  code: 'ERR_ASSERTION'
# pass 0
# fail 1

Step 4 - gate restored, hardened test passes

=== normal ===
ok 1 - a partition already due for compaction skips the resettle-candidate row scan
# pass 1
# fail 0
=== under external truncation (guard 1 defeats it) ===
ok 1 - a partition already due for compaction skips the resettle-candidate row scan
# pass 1
# fail 0

Stacking

This stacks on #706 and should merge after it. The test being hardened was added by #706 (fix/issue-700-followup) and is not on master; this branch is based on #706's head 47fd4a4, so the fix applies to the code it targets. The diff here is one commit touching only the test file.

Gate

  • npm test: 3907 tests, # pass 3903, # fail 0 (6 pre-existing skips)
  • npm run typecheck: clean
  • npm run smoke -- cache_lifecycle_maintenance: ok
  • npm run smoke -- incremental_sink_compaction: ok
  • npm run smoke -- cache_roundtrip: ok

Fixes #707

test and others added 4 commits August 10, 2026 23:53
… MaintenanceReport symmetry, span visibility, LLP gloss

PR #701 was squash-merged at head 287b67b, before the round-2 review fixes
in cc82d6f were pushed, so four verified fixes never reached master.

- Hoist a cheap `compactionDue` check above the `hasResettleCandidate` row
  scan and gate the scan on `!compactionDue`. Recognition of a foreign
  sorted `replace` outranks the resettle check, so the first tick after
  each foreign replace paid a complete single-column scan of the day
  purely to discard the answer.
- Add `totalRebaselined` to `MaintenanceReport` beside `totalCompacted`,
  and let `query.js` read it instead of re-deriving the count.
- Tag the enclosing `maintenance.partition` span with `rebaselined`; the
  `hyp_rebaselines` counter carries only the dataset, not the partition.
- Give LLP 0199's bare `Extended-by: LLP 0207` the corpus's linked and
  glossed form.

Co-Authored-By: Claude <noreply@anthropic.com>
Round-1 review of #706 approved the carry but flagged that none of the
four stranded fixes was pinned by a committed test, plus two @ref nits.

- Add three tests to test/core/cache-retention-maintenance.test.js, in
  the foreign-sorted-replace block:
  - `totalRebaselined === 1` on a re-baselining run, and `=== 0` once
    converged (pins the MaintenanceReport symmetry fix).
  - a capturing TracerProvider asserting the maintenance.partition span
    carries `rebaselined: true` (pins the span-attribute fix).
  - a partition already due for compaction: assert the resettle
    candidate's data file is read once, not twice, by spying on
    `fs.readFileSync` (pins the scan-skip fix). hasResettleCandidate's
    return value is otherwise unobservable once compactionDue is true
    (it's discarded via `||`), so this is the cheapest honest signal
    available; each new test was verified to fail when its
    corresponding fix is reverted.
- Retarget the scan-skip gate's @ref from LLP 0207#foreign-replace to
  #outranks-resettle: the gloss ("recognition ... still outranks it")
  is that anchor's actual subject, not the recognition test's.
- Drop the @ref on the span-attribute comment: #re-baseline settles the
  cursor-write shape, not telemetry, so citing it was close to
  mechanical. The prose rationale stays; it's the useful part.
- Amend the PR body's claim about item 1: `needsCompaction` (pure,
  read-only) is now evaluated unconditionally in the re-settle path
  where the old `||` short-circuited past it, so more moved than "only
  the scan's side effect is skipped."

Co-Authored-By: Claude <noreply@anthropic.com>
- test/core/cache-retention-maintenance.test.js: the resettle-scan-skip
  test asserted a global .parquet readFileSync count of 1 for the whole
  maintainCache tick, not just reads the gate governs. Any future second
  legitimate read elsewhere in the tick would break it with a message
  that misdirects the next reader. Switch to capturing a stack trace per
  .parquet read and asserting none pass through hasResettleCandidate,
  attributing each read to its caller instead of counting tick-wide.
  Verified both directions: passes at this head (3x, no flake), and
  fails with the expected message when the !compactionDue && gate in
  src/core/cache/maintenance.js is reverted, showing hasResettleCandidate
  in the offending stack.
- PR body: item 2's exemplar list cited llp/0012:9, 0017:9, 0036:9,
  0041:8, 0191:9 as using the linked-and-glossed Extended-by form,
  but only 0191 actually carries a link; the other four are gloss-only.
  Replaced with docs confirmed to use the linked form: llp/0106:9,
  0129:9, 0158:9, 0180:9, 0182:9, 0188:9, 0190:9, 0191:9. (0201, also
  suggested, only has a backward "Extends" pointer, not "Extended-by",
  so it was excluded.)

Co-Authored-By: Claude <noreply@anthropic.com>
`a partition already due for compaction skips the resettle-candidate row
scan` attributes each `.parquet` read to its caller by capturing
`new Error().stack` inside a `readFileSync` mock and asserting no stack
passes through `hasResettleCandidate`. The discriminating frame sits at
frame 8 of 10, and `Error.stackTraceLimit` defaults to 10. Three more
frames anywhere between the mock and the caller (an icebird refactor,
extra node:test mock internals, a wrapper in `resolver.js`) drop that
frame, and a negative "no stack mentions X" assertion then passes
vacuously: the test stays green even with the `!compactionDue &&` gate
reverted. The `stacks.length > 0` sanity check only proved a read
happened, not that the stacks were attributable, and the
`new Error().stack ?? ''` fallback would store an unattributable empty
string without complaint.

Demonstrated by running with `Error.stackTraceLimit = 7` (standing in for
the three extra frames) and the gate reverted: the old test passed.

Two guards, because they cover different routes to the same blindness:

1. Raise `Error.stackTraceLimit` to 50 while the mock is installed,
   restoring the previous value in the existing `finally` so it is put
   back even if the test throws. This removes the practical hazard.
2. Assert positively that some captured stack names `compactGeneration`,
   the legitimate reader. It calls `scanRowsFromTable` from exactly the
   same depth as `hasResettleCandidate` does, so any truncation deep
   enough to hide the frame the negative assertion hunts for also hides
   this one and fails the test loudly. Asserting on `scanRowsFromTable`
   itself would not work: it sits one frame shallower and survives
   truncation that has already blinded the real check. This guard also
   catches the empty-string fallback.

Under the same truncation with the gate still reverted, the hardened
test fails; with the gate restored it passes, truncated or not.

No production code changed.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review round 1 - head 4045b63

Verdict: approve. No findings. Every claim in the PR description was reproduced exactly rather than accepted: the defect was real, both guards work, and nothing leaks.

Scope

git diff 47fd4a4..4045b632 -- src/ is empty. The sole commit touches only test/core/cache-retention-maintenance.test.js (+26 lines, no deletions). The large diff against master is entirely the stacked #706 chain, as stated. The existing @ref LLP 0207#outranks-resettle still resolves and no new refs were added.

The four-step demonstration, re-derived

With the gate reverted (maintenance.js:301, !compactionDue && settle back to settle):

step test truncation gate result
0 old (47fd4a4) none reverted fails, at async hasResettleCandidate present
1 old stackTraceLimit = 7 via NODE_OPTIONS --import reverted passes - blindness reproduced
3 new external 7 reverted fails on the attribution assertion
3b new, in-test limit forced to 7 n/a reverted fails on the new sanity assertion
3b' new, in-test limit forced to 7 n/a restored fails loudly - guard 2 is gate-independent, as intended
4 new none, then external 7, then real --stack-trace-limit=7 restored passes in all three

Step 0's stack is exactly 10 frames with hasResettleCandidate at frame 8, so the comment's "2 frames of headroom, 3 extra frames drops it" arithmetic is exact rather than approximate. Step 1 is the load-bearing one: the old test really does go green under truncation with the gate reverted, so the defect this PR fixes was genuine.

The scanRowsFromTable call, verified independently

Dumping the good-path stacks gives 15 frames with scanRowsFromTable at 7 and compactGeneration at 8 - exactly the depth hasResettleCandidate occupies on the bad path, with scanRowsFromTable one frame shallower in both. So the author's non-obvious call is right: asserting on scanRowsFromTable would survive truncation to 7 that has already blinded the negative check, reproducing the very blindness the fix removes. Choosing compactGeneration is load-bearing, not stylistic.

Restoration, vacuity and cost

  • originalStackTraceLimit is captured before the try, the raise is inside it, and the restore is the first statement of finally, ahead of the await fs.rm - so a failing cleanup cannot skip it. Verified empirically by injecting a forced assertion failure mid-test and probing from a later test: the limit was back to 10.
  • node:test runs top-level tests in a file with concurrency 1, so the raised global is never visible to a sibling even while in effect, and nothing else in the file touches Error.stackTraceLimit or .stack.
  • stacks.some(...) on an empty array is false, so the new assertion cannot pass vacuously; with the existing stacks.length > 0 ahead of it, the ?? '' empty-string hole really is closed.
  • No measurable cost: 5 runs each, old 23.4-26.6 ms vs new 22.7-23.5 ms. The gate means one .parquet read per run and the full stack is 15 frames, well under 50.

Gates

npm test 3903 pass / 0 fail / 6 skipped, typecheck clean, smokes cache_lifecycle_maintenance, incremental_sink_compaction, cache_roundtrip all ok. Target file run 5 times consecutively: 38/38 each, no flake. No em dashes, no code semicolons, and the comment prose matches both guards and the measured frame depths.

Note for whoever merges: this PR is stacked on #706 and should land after it.

@philcunliffe
philcunliffe marked this pull request as ready for review August 11, 2026 02:50
@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-up: deferred review findings from PR #706

1 participant